-
Notifications
You must be signed in to change notification settings - Fork 5
Implement VotingStreakMultiplier #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs slight redesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pattern is implemented as expected! looking very elegant!
@bagelface @RonTuretzky with my latest commit here, the build is compiling without errors/warnings and the tests all pass. Can you advise on next steps? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far , please add a VotingStreakMultiplier.t.sol
file with unit tests
test/YieldDistributor.t.sol
Outdated
@@ -451,6 +454,190 @@ contract YieldDistributorTest is Test { | |||
} | |||
} | |||
|
|||
contract VotingStreakMultiplierTest is YieldDistributorTest { | |||
// uint256 constant START = 32_323_232_323; | |||
uint256 constant MULTIPLIER_INCREMENT = 0.02e20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's up with this number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have a few readability and style comments, but nothing that would prevent this from being merged in. Great stuff!
…hould be set to fixed point notation small percentages
src/VotingMultipliers.sol
Outdated
@@ -71,8 +80,8 @@ contract VotingMultipliers is Ownable2StepUpgradeable, IVotingMultipliers { | |||
/// @param _user The address of the user | |||
/// @param _multiplierIndexes Array of multiplier indexes to use | |||
/// @return The total multiplier value for the user | |||
function getTotalMultipliers(address _user, uint256[] calldata _multiplierIndexes) public view returns (uint256) { | |||
uint256 _totalMultiplier = 0; | |||
function getTotalMultipliers(address _user, uint256[] calldata _multiplierIndexes) public returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get functions should have the view visibility. this won't be very useful on the frontend as is. i would remove updateMultiplyingFactor
to allow this to be view. then just tolerate that it might be out of sync on the frontend. maybe add a "isSynced" flag based on timestamps and cycle length that can be checked and return alongside the multiplier value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bagelface this function gets called from the yielddistributor, but the following function of the same name has a @dev
notice that its intended to be called from front end, and is a view
function.
so my sense is this should remain as is, but please clarify if you see it differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My issue with this change is mostly from a readability perspective. The function name is getTotalMultipliers
but it is not a view function, which is inconsistent. The visibility should either be updated or the name of the function should change so that it's clear that state changes are being made when it's called.
/// @dev Implements IMultiplier and IVotingStreakMultiplier interfaces | ||
contract VotingStreakMultiplier is Initializable, OwnableUpgradeable, IMultiplier { | ||
/// @notice The base multiplier value (100% in fixed-point representation) | ||
uint256 constant BASE_MULTIPLIER = 1e18; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually good to be explicit with accessibility. this will default to private
@@ -2,7 +2,11 @@ | |||
pragma solidity ^0.8.0; | |||
|
|||
interface IMultiplier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually might be a good idea to just add BASE_MULTIPLIER
constant here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had this instinct as well, but found that solidity prevents variables from being set in interfaces. cursor suggested i create a library directory and define it, like MultiplierConstants, and import it from there. Do you think that's a good solution ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did this in a separate commit so its clearer f17990d - wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh weird I didnt know that. I do think this makes things a little easier to work with so that the value doesn't need to be explicitly defined each time
// Expected voting power = initial * boost | ||
// Start with base 100% (1e18) and add only the bonus amounts | ||
uint256 totalMultiplier = 1e18; // Base 100% | ||
totalMultiplier += (1.5e18 - 1e18) + (2e18 - 1e18); // Add bonuses from both multipliers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just heads up, you can use the ether
keyword to represent this like:
totalMultiplier += (1.5 ether - 1 ether) + (2 ether - 1 ether);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im going to choose to leave it like 1e18 only because i think thats a little more self explanatory than ether
, and i havent gotten the sense you have a strong preference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to know tho, appreciate the tip!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few comments, but lgtm
When a vote is cast (via the YieldDistributor), the VotingStreakMultiplier updates it's state variables tracking user's multiplier value and validity.
When this contract is deployed, the front end can call
getMultiplyingFactor
for the user's current multiplier.